[Peek] Fix memory leak caused by unmanaged bitmaps not being freed #34484
Conversation
|
@microsoft-github-policy-service agree |
| @@ -103,12 +103,6 @@ | |||
| </Page> | |||
| </ItemGroup> | |||
|
|
|||
| <ItemGroup> | |||
There was a problem hiding this comment.
Seems like unrelated changes here. Is this needed?
There was a problem hiding this comment.
This was me removing a duplicate entry I found in the project file, and it's not related to the memory leak issue. I can split it out into a separate PR if you'd prefer.
There was a problem hiding this comment.
LGTM!
Tested the fix and also performed a sanity check of Peek.
Based on what the fix was, I still don't have a good explanation for what could cause this behaviour we observed earlier, but maybe that's not critical to address now.
It would be good to make the ImagePreviewer::Dispose workflow idempotent, as that is a requirement of IDisposable. Currently, calling Dispose multiple times would result in multiple deletes of native handles. But that does not seem to be happening in practice, so we can defer that for later.
|
Thank you for approving the merge, @drawbyperpetual ! I can speak to the strange behaviour you were seeing previously, where the increase in private bytes used was not as prevalent when the same files were previewed again: this could be because of the thumbnail retrieval code in Peek and the relationship with the Windows thumbnail cache. You are 'priming' the cache by previewing the images, as Peek always calls the Shell thumbnail retrieval/creation routine for every image, even when the full size image loads correctly. This was the fundamental 'tell' of the memory leak issue, as the thumbnail bitmaps are unmanaged and never freed. I've recently raised an issue about the unnecessary thumbnail generation as #34490 . In the majority of cases in my testing, the bitmaps are never used, so the #34490 fix actually saves a considerable amount of memory use and subsequent garbage collection (75%+ GC reduction). I totally agree about the existing |
|
@daverayment as FYI, we've been heads down trying to get ship .84 and stabilize based on feedback. We appreciate the PRs here! |
|
@crutkas Thanks! It's a privilege to be able to contribute, and there's certainly no rush. Best of luck with the upcoming release 😀 |
|
Thanks a lot for the contribution! Merging this in! |
Summary of the Pull Request
Fixes the Peek PowerToy leaking unmanaged resources associated with bitmaps when using the
ImagePreviewer. Also ensures any Previewer implementingIDisposablewill be correctly disposed.PR Checklist
Detailed Description of the Pull Request / Additional comments
The Peek PowerToy creates a Previewer for each item as the user navigates a folder. For the ImagePreviewer, a
Clearmethod is provided which cleans up unmanaged resources, including bitmaps created for preview thumbnails. Unfortunately,Clearis not called on the old Previewer before a new instance is created, leading to unreachable resources which are never freed. This PR ensuresDisposeis called on a Previewer if it implementsIDisposable. ImagePreviewer callsClearwithin itsDispose, which fixes the leak.Validation Steps Performed
Manual validation:
PowerToys.Peek.UIexe in VS, where the memory leak manifests.Byte[]instances were present in the heap snapshot.Before Fix
Despite garbage collection, unmanaged resources are not freed, leading to an ever-increasing amount of memory used - nearly 2GB in this example.
After Fix